-
Notifications
You must be signed in to change notification settings - Fork 381
feat(lambda-events): Improve ergonomics of SqsBatchResponse and KinesisEventResponse
#1063
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
SqsBatchResponse
SqsBatchResponseSqsBatchResponse and KinesisEventResponse
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The external API seems fine to me. Though, need to get consensus with maintainers about both having codegenned builders and these sort of extra-ergonomic constructors as discussed in #1060
Left some feedback around making this more maintainable using ..Default::default() constructor syntax since we are crate-local.
We probably should also explicitly state in doc comments that all fields besides the one being set, will use the defaults for the struct (which will generally be None, an allocation-free empty string or map, etc, though that doesn't need to go in doc comments).
| /// | ||
| /// # Example | ||
| /// | ||
| /// ```rust,no_run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: any reason this needs to be no_run?
| /// | ||
| /// # Example | ||
| /// | ||
| /// ```rust,no_run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: any reason this needs to be no_run?
| /// } | ||
| /// ``` | ||
| pub fn add_failure(&mut self, message_id: impl Into<String>) { | ||
| self.batch_item_failures.push(BatchItemFailure { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason not to use ..Default::default() here to reduce churn in this function as the struct evolves? We shouldn't be limited by #[non_exhaustive] locally to the crate?
Ie:
self.batch_item_failures.push(BatchItemFailure {
item_identifier: message_id.into(),
...Default::default()
};
We also should state in the doc comment that defaults will be used for all fields besides item_identifier.
| { | ||
| self.batch_item_failures = message_ids | ||
| .into_iter() | ||
| .map(|id| BatchItemFailure { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same feedback using ..Default::default() for future proofing, mentioning that defaults are used for other fields.
| /// to be enabled in your Lambda function's Kinesis event source mapping configuration. | ||
| /// Without this setting, Lambda will retry the entire batch on any failure. | ||
| pub fn add_failure(&mut self, item_identifier: impl Into<String>) { | ||
| self.batch_item_failures.push(KinesisBatchItemFailure { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same feedback using ..Default::default() for future proofing, mentioning that defaults are used for other fields.
| /// **Important**: This feature requires `FunctionResponseTypes: ReportBatchItemFailures` | ||
| /// to be enabled in your Lambda function's Kinesis event source mapping configuration. | ||
| /// Without this setting, Lambda will retry the entire batch on any failure. | ||
| pub fn set_failures<I, S>(&mut self, item_identifiers: I) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same feedback using ..Default::default() for future proofing, mentioning that defaults are used for other fields.
|
Update - I spoke with other maintainers, and we're happy to accept this contribution with the above comments addressed! Thanks for this :) @PartiallyUntyped will follow up in #1060 about the broader plans around codegenned builders, how they relate to these fine-tuned constructors, etc, but we don't need to let that block this PR. |
|
Looks like some CI failures as well due to the new doc tests. I don't mind adding It's also fine to rip out the usage samples if you prefer, though I do think they are pretty useful. I think it will keep requiring that I approve the CI runs for you, which I am happy to, but you should be able to repro these failures on your fork as well to validate a fix. |
📬 Issue #, if available:
non_exhaustive) #1060✍️ Description of changes
This PR improves the ergonomics of working with
SqsBatchResponseandKinesisEventResponse.Since v1.0, these structs have been marked as
#[non_exhaustive], which means youcan no longer construct them and provide their content in a single expression.
Note
This is also the case for the related structs
SqsBatchItemFailureandKinesisBatchItemFailure.❌ Previous (pre-1.0, now broken) API
The following code used to be valid before 1.0, but no longer compiles:
With
#[non_exhaustive], you now need something slightly more verbose, similar tothe official example in the repo
([advanced-sqs-partial-batch-failures](https://github.com/aws/aws-lambda-rust-runti
me/blob/main/examples/advanced-sqs-partial-batch-failures/src/main.rs)):
This works, but it is more verbose and a bit less ergonomic for a very common usage
pattern.
✅ Proposed (this PR): ergonomic helper API
This PR adds two helper methods to both
SqsBatchResponseandKinesisEventResponse:add_failure(item_identifier)- Add a single failure to the responseset_failures(item_identifiers)- Set multiple failures at once (replacesany existing failures)
Both methods accept flexible input types via
impl Into<String>, so you can pass&str,String, or any other compatible type.SQS Example using
add_failure:SQS Example using
set_failures:Kinesis Example using
add_failure:This keeps the types
#[non_exhaustive]while restoring a convenient anddiscoverable way to build partial batch responses.
🔏 By submitting this pull request
cargo +nightly fmt.cargo clippy --fix.documentation.
license.